Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add wait_for_docker_container helper #235

Merged
merged 1 commit into from
Jan 17, 2024
Merged

feat: Add wait_for_docker_container helper #235

merged 1 commit into from
Jan 17, 2024

Conversation

TheoD02
Copy link
Contributor

@TheoD02 TheoD02 commented Jan 11, 2024

Hey

Following the implementation of the wait_for feature (#216), here's what's next for the docker container that was planned as second PR

The system is more or less the same as before, but more relying on existing helpers.

I've left the autostart feature, so let me know if it's more suitable than the one provided in the first place. If it doesn't suit the need then I'll delete it :)

@TheoD02
Copy link
Contributor Author

TheoD02 commented Jan 11, 2024

There's going to be a bit of a synchro problem when generating the tests in this case, because we won't necessarily have the service to check potentially on our workstation, and for the moment not in the CI either.

How can we overcome this problem?

Add a lightweight service that allows a check to be made in the ci.yml?
How can we manage it locally?
Launch a service directly from the task? Like `run('docker run -d service');

Waiting your response 😄

function wait_for_docker_container_task(): void
{
try {
run('docker run -d --rm --name helloworld alpine sh -c "echo hello world ; sleep 10"', quiet: true);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is OK for CI and local ? quiet is used to prevent showing the container ID that is random on each test generation run

@lyrixx
Copy link
Member

lyrixx commented Jan 15, 2024

Hello,

Thanks for this contribution.

  • This should not be the responsibility of a wait_for function to start something. So could you remove every bits about the starts feature?
  • I think the code could be simplified. And, there a double writing to the output. I refactored your previous code to avoid such situation. IMHO, only wait_for() should output something:
    • Could you try to remove all "writing" stuff (wait_for()) should do it
    • Simplify the code. Example with waitForDockerContainer::waitForDockerContainer => port checking, I would expect something like:
      foreach ($portsToCheck as $port) {
          $this->waitForPort(io: $io, port: $port, timeout: $timeout, quiet: true);
      }
  • rebase
  • Squash all you commits

Thanks

@TheoD02
Copy link
Contributor Author

TheoD02 commented Jan 16, 2024

Hey !

I've corrected the feedbacks, I'm unfamiliar with working with commits squash I don't know if I've missed something?

CS seems to break on a file I didn't modify and locally I have no problem to fix

For PhpUnit, it's the same problem that we've already seen: the test time needs to be reduced, need to do here or in another PR ?

@lyrixx
Copy link
Member

lyrixx commented Jan 16, 2024

I've corrected the feedbacks, I'm unfamiliar with working with commits squash I don't know if I've missed something?

I'll take care of it while merging. No worries :)

CS seems to break on a file I didn't modify and locally I have no problem to fix

wahoo, it's all broken indeed! It looks like the latest php-cs-fixer release is broken. Don't bother with that

ref: PHP-CS-Fixer/PHP-CS-Fixer#7753

EDIT this should be solved

For PhpUnit, it's the same problem that we've already seen: the test time needs to be reduced, need to do here or in another PR ?

I noticed that too. since we rely on network, it's broken sometime. I'll submit a PR for that.

Copy link
Member

@pyrech pyrech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer this new implementation, thanks for the modifications 🙂

@pyrech
Copy link
Member

pyrech commented Jan 17, 2024

@TheoD02 I just rebased your PR and remove the exception class that was not used

@TheoD02
Copy link
Contributor Author

TheoD02 commented Jan 17, 2024

@TheoD02 I just rebased your PR and remove the exception class that was not used

@pyrech Thank for the fix of the last remaining tasks 💯😄

@lyrixx
Copy link
Member

lyrixx commented Jan 17, 2024

@TheoD02 I have rebased and tweaked a bit your PR. Thanks for the work !

I'm gonna merge it 🎉

@lyrixx lyrixx merged commit 17ea578 into jolicode:main Jan 17, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants